feat(api): pin the v2 HTTP contract and reject unknown request fields - #242
Conversation
…chema Two of the three seams every PostGuard client depends on, as machine-checkable artifacts (EPIC #201, issues #209/#210): - pg-pkg/api-description.yaml: the full v2 OpenAPI (mirrors cryptify's api-description.yaml pattern) — endpoints, auth modes, request/response schemas incl. the ConItem condiscon shape, error envelope, rate limits. Lints clean (redocly). - pg-pkg/schemas/irma-auth-request.schema.json + canonical examples: the condiscon request pinned as JSON Schema, deliberately faithful to serde behavior (unknown fields ignored — documented). Examples cover the legacy flat shape, nested disjunctions, the optional empty-conjunction convention and its irmamobile#360 ordering workaround, plus the classic nesting mistakes as invalid vectors. - pg-pkg/tests/contract_examples.rs makes the examples EXECUTABLE: every valid example must parse as IrmaAuthRequest, every invalid one must be rejected — so the examples and the parser cannot drift apart silently. The e2e harness (postguard-e2e) will additionally validate live responses against the OpenAPI and the examples against the schema (issue #212).
|
Thanks for the PR, @rubenhensen! I'm on it — kicking off a review of the OpenAPI description and JSON Schema additions (closing #209 and #210) for correctness and rule-compliance, then I'll post a single consolidated review with a clear verdict. |
![dobby-coder[bot]](https://avatars.githubusercontent.com/in/3365457?s=60&v=4){kind=small}
There was a problem hiding this comment.
Thorough contract-pinning PR. The executable-examples approach (valid/ must parse, invalid/ must be rejected, with #212 closing the schema ↔ parser ↔ server loop) is a solid way to keep the seam honest. Tests pass (pg-pkg 57 + contract_examples 2) and redocly lints clean apart from the 4xx warnings noted below.
One blocking issue and two smaller notes, all verified against the handler source:
- bug:
/v2/request/jwt/{token}documents 502, but the server returns 503 on the upstream-error path (jwt.rs:21UpstreamError ->error.rs:83SERVICE_UNAVAILABLE; there is no 502 anywhere in the PKG). #212 validates live responses against this spec and would trip on the mismatch. The statusevents operation just below already documents 503 for the same error. - style:
jwtandstatuseventsboth return 400 for a malformed token (each has a passing*_rejects_malformed_token_with_400test) and 500 on an unreachable upstream; neither is documented. Two of the eight redocly warnings are a genuine missing 4xx, not house-style. - nit: the empty-alt-LAST note contradicts the server's own
optional:trueexpansion, which emits the empty conjunction first (start.rs:60).
Rule check (conventional-commit title, closing keywords, cargo-fmt, prose) is clean: the docs(pkg): title satisfies pr-title.yml, Closes #209 / closes #210 link correctly, and EPIC #201 is left open. The 502->503 fix and the missing 400 are small, concrete edits.
| schema: | ||
| type: string | ||
| description: "A compact JWS (three dot-separated segments)." | ||
| "502": |
There was a problem hiding this comment.
Wrong status code. /v2/request/jwt/{token} documents 502 here, but the handler maps this path to 503: jwt.rs:21 does .error_for_status().or(Err(Error::UpstreamError))?, and error.rs:83 maps UpstreamError -> SERVICE_UNAVAILABLE (503). There is no 502 path anywhere in the PKG, and the /statusevents/{token} operation just below correctly documents 503 for the same UpstreamError. Since #212 will validate live PKG responses against this spec, a real unknown-session lookup returns 503 and would mismatch the documented 502. Change this to "503".
| description: "The session token from `/v2/request/start`." | ||
| schema: | ||
| type: string | ||
| responses: |
There was a problem hiding this comment.
Missing documented error responses. Both jwt and statusevents validate {token} with is_valid_session_token and return Error::SessionTokenInvalid -> 400 for a malformed token (jwt.rs:11-13, statusevents.rs:25-27; each has a passing *_rejects_malformed_token_with_400 test), plus Error::Unexpected -> 500 when the upstream is unreachable. Neither 400 nor 500 is documented on these two operations. redocly's operation-4xx-response warnings [7]/[8] point at exactly these operations, so the PR description's claim that the remaining lint warnings are only house-style (license field, localhost server) isn't quite accurate: two of the eight flag a genuine missing 4xx. Please add the 400 (and ideally 500) responses here and on statusevents.
| }, | ||
| "conjunction": { | ||
| "title": "Conjunction", | ||
| "description": "An AND of attributes. An EMPTY conjunction inside a disjunction marks the whole disjunction as skippable (Yivi's optional-disjunction convention). Ordering note (irmamobile#360): deployed Yivi apps mis-render a disjunction whose empty alternative comes FIRST, so clients currently place the empty alternative LAST; the server accepts either order.", |
There was a problem hiding this comment.
nit (pre-existing, not introduced by this PR). This note, and the identical one at api-description.yaml:402, tells clients to place the empty alternative LAST as an irmamobile#360 workaround (apps mis-render an empty-first alternative). But the server's own optional:true expansion emits the empty conjunction FIRST: start.rs:60 vec![vec![], vec![ar]], with a test asserting dis[0].is_empty() ("empty alternative first"). If the #360 mis-render is real, the server-generated optional discon trips the same bug. The new contract docs make the contradiction explicit, so it's worth either scoping the note to client-supplied discons or reordering start.rs.
|
Reviewed this and posted a request changes review with 3 inline comments:
Everything else — commit title, closing keywords, formatting, prose — checked out clean, and the executable valid/invalid examples plus the #212 loop are a nice way to keep this contract honest. Details in the review: #242 (review) |
In a key-issuance request, a silently ignored misspelled field (`vaule` for
`v`, `optioanl` for `optional`) widens the disclosure the caller intended —
intent-weakening must be a 400, not a reinterpretation. Deployment reality
allows going strict directly: all clients are first-party.
- deny_unknown_fields on IrmaAuthRequest, DisclosureAttribute and
SigningKeyRequest. identity::Attribute stays lenient deliberately: it is
shared with the bincode wire format, the wasm/FFI seal boundary and
response policies, and its values are not load-bearing in request paths.
- ConItem's untagged Deserialize replaced with a manual visitor: the JSON
shape (object vs array) discriminates the variants, so the caller gets the
precise inner error ("unknown field `vaule`, expected one of `t`, `v`,
`optional`") instead of "did not match any variant".
- Body-deserialization failures now use the documented {error, message}
envelope (JsonConfig error_handler) instead of actix's plain-text default.
- Schema/OpenAPI flipped to additionalProperties: false; examples stripped of
$comment (which only ever parsed thanks to the old leniency — commentary
moved to examples/README.md); new executable invalid vectors for the typo
cases; tests assert the 400 names the offending field.
rubenhensen
changed the title
…sevents response docs
The contract docs surfaced a contradiction: clients place a discon's empty
alternative LAST to dodge irmamobile#360 (apps mis-render empty-first), but
the server's own optional:true expansion emitted it FIRST — tripping the very
bug the workaround exists for. The expansion now appends it last; either
order is still accepted on input, and the schema/OpenAPI notes say so.
Also per review: /v2/request/jwt/{token} maps upstream error statuses to 503
(UpstreamError), not the previously documented 502, and both jwt and
statusevents additionally return 400 (malformed session token) and 500
(upstream unreachable) — now documented.
|
All three addressed, /dobby:
|
|
Thanks for the update — I'm on it. Starting my review of the PR now, focusing on the JWT 502→503 fix, the new 400/500 documentation, and especially the server-side reordering of the empty alternative in |
The #360 mis-render of empty-first alternatives is fixed and shipped (irmamobile PRs #557/#558, released in v8.1.0 on 2026-07-10; issue closed 2026-07-14), and postguard-website has already reverted to the conventional leading-empty order. Prescribing empty-last in the new contract docs — and flipping the server's optional expansion to match — was codifying a workaround for a dead bug. - start.rs: optional expansion back to the conventional empty-first - schema/OpenAPI/api.rs: ordering documented as presentational with either order accepted; #360 kept only as a one-line historical note - examples: condiscon-name-disjunction.json now mirrors the website's actual current request verbatim (leading empty alternative, four name sources, optional phone/dob); optional-discon example renamed and reordered
|
Correction to my earlier '#360 workaround' change, prompted by review discussion: the workaround is obsolete and is now removed rather than codified. Verified timeline: fix merged upstream (irmamobile #557/#558, June 12/15), shipped in v8.1.0 (July 10), issue closed July 14 — and postguard-website has already reverted to the conventional leading-empty order in So the previous commit had it backwards (it moved the server toward the obsolete order). Now: the server's |
There was a problem hiding this comment.
Solid contract-pinning PR — the parser change is well-reasoned (a silently-dropped vaule widening a disclosure is exactly the kind of leniency a key-issuance API should not have), and the executable valid//invalid/ vectors plus the end-to-end 400-envelope test make it hard to regress. The three points from the last round are all addressed, and the empty-alternative fix (emit the empty option last, per irmamobile#360) is correct and now matches the client-side workaround.
Two non-blocking notes before merge:
-
PR title / release-plz semver.
deny_unknown_fieldsonIrmaAuthRequest/DisclosureAttribute/SigningKeyRequestis a genuine client-observable break — you already say so in the "Breaking note". Per this repo's CLAUDE.md, release-plz decides the bump from the squash-merge subject (the PR title), and a break needsfeat(api)!:plus aBREAKING CHANGE:footer; without the!this ships as a minor bump with no changelog warning. Recommend retitling tofeat(api)!:and adding the footer so the release reflects the break. -
One stale doc comment on
IrmaAuthRequest(inline).
Neither blocks — this is good to merge once the title reflects the break.
| @@ -45,6 +51,7 @@ pub struct DisclosureAttribute { | |||
| /// `[{t,v?,optional?}, ...]` JSON shape still deserialises, because | |||
| /// `ConItem` is `#[serde(untagged)]`. | |||
| #[derive(Debug, Serialize, Deserialize)] | |||
There was a problem hiding this comment.
Stale after this PR: the comment says the flat shape still deserialises because ConItem is #[serde(untagged)], but this PR replaces the untagged deserialize with a manual Deserialize impl (the visit_map/visit_seq visitor below) and drops Deserialize from ConItem's derive. #[serde(untagged)] now governs only Serialize. Anyone debugging a deserialize error will be misdirected — point this at the manual impl instead (which is what actually keeps the flat {t,v?,optional?} shape parsing, via visit_map).
| /// `ConItem` is `#[serde(untagged)]`. | ||
| #[derive(Debug, Serialize, Deserialize)] | ||
| #[serde(deny_unknown_fields)] | ||
| pub struct IrmaAuthRequest { |
There was a problem hiding this comment.
This deny_unknown_fields (and the ones on DisclosureAttribute and SigningKeyRequest) is a client-observable breaking change: requests that previously succeeded with an extra/misspelled field now 400. That is the right behaviour, but per CLAUDE.md release-plz derives the version bump from the PR title, and feat(api): (no !) ships this as a minor bump with no BREAKING CHANGE: changelog entry. Recommend feat(api)!: + a BREAKING CHANGE: footer so the release matches the Breaking note in the PR body.
|
Nice, all three check out cleanly against HEAD — the 503 mapping, the 400/500 docs on |
The gate as reviewed promised more than fail-on: ERR delivers. Measured against oasdiff v1.26.1 on the real spec, ERR silently passed six changes COMPATIBILITY.md forbids: a changed status code, a dropped response enum value, a removed and a renamed optional response property, a removed non-success status, and a removed request parameter. Two of those (response-non-success-status-removed, response-property-enum-value-removed) rate ERR but are opt-in, so they only run when named in include-checks. The rest are WARN, and on this spec that is most of the "no removing a route or a field" rule: only `status` is ever `required`, so `key`, `proofStatus`, `pubSignKey` and the rest are optional. So the gate now runs fail-on: WARN plus those two checks. Of the 31 WARN checks, response-property-enum-value-added is the only one that fires on something COMPATIBILITY.md did not already forbid, so that document now names a new response enum value as non-additive too. pg-pkg/tests/api_gate.rs is the regression test: it mutates the spec 18 ways, runs the engine with the flags the action's entrypoint builds, and asserts which mutations the gate stops. It fails on the old settings with all six gaps named, and skips when oasdiff is not on PATH, which is the case in CI. Refs #249, #242. Part of #247 (workstream C). Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
* ci: breaking-change gate on the pg-pkg OpenAPI contract (oasdiff) The gate itself is an oasdiff job in .github/workflows/api-diff.yml, which the App cannot push, so the YAML ships as a comment on this PR for a maintainer to apply. What is committed here is the docs half: COMPATIBILITY.md and CLAUDE.md now say the /v2 rules are still a review rule, name the job and where its YAML is waiting, and record how to reproduce the verdict locally (oasdiff v1.26.1, the version the pinned action runs). Refs #249. Part of #247 (workstream C). * ci: raise the oasdiff gate to WARN and pin its verdicts in a test The gate as reviewed promised more than fail-on: ERR delivers. Measured against oasdiff v1.26.1 on the real spec, ERR silently passed six changes COMPATIBILITY.md forbids: a changed status code, a dropped response enum value, a removed and a renamed optional response property, a removed non-success status, and a removed request parameter. Two of those (response-non-success-status-removed, response-property-enum-value-removed) rate ERR but are opt-in, so they only run when named in include-checks. The rest are WARN, and on this spec that is most of the "no removing a route or a field" rule: only `status` is ever `required`, so `key`, `proofStatus`, `pubSignKey` and the rest are optional. So the gate now runs fail-on: WARN plus those two checks. Of the 31 WARN checks, response-property-enum-value-added is the only one that fires on something COMPATIBILITY.md did not already forbid, so that document now names a new response enum value as non-additive too. pg-pkg/tests/api_gate.rs is the regression test: it mutates the spec 18 ways, runs the engine with the flags the action's entrypoint builds, and asserts which mutations the gate stops. It fails on the old settings with all six gaps named, and skips when oasdiff is not on PATH, which is the case in CI. Refs #249, #242. Part of #247 (workstream C). Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> * docs: say the api-diff workflow is not committed yet in the test header The test's module docs described the gate in the present tense, which is the same overpromise the reviewed docs had: the workflow YAML is still in a PR comment waiting for workflows: write. CLAUDE.md and COMPATIBILITY.md already say so; this file now does too. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> * test(pg-pkg): pin that the gate stops a new response enum value COMPATIBILITY.md gained "adding a value to a response enum" for this gate, and it was the one rule api_gate.rs did not cover. It holds only at WARN (measured on oasdiff v1.26.1: exit 0 at ERR+include-checks, exit 1 at WARN+include-checks), so a revert to fail-on: ERR dropped it with the test still green -- exactly the drift the test exists to catch. At ERR the test now reports four wrong verdicts instead of three, the fourth being this one. * ci: run the pg-pkg OpenAPI breaking-change gate Dobby's YAML from the #269 comment, applied verbatim (the App cannot push workflows). Landing it here rather than on main so the gate self-tests on this PR, and tightening the two doc lines that described it as not-yet-live. * test(pg-pkg): pin the workflow's oasdiff inputs against the test's constants api_gate.rs's header named a hole that only existed while the YAML was uncommitted: FAIL_ON and INCLUDE_CHECKS are what the test runs oasdiff with, the workflow step is what CI runs it with, and nothing compared the two. The verdict test skips on every runner, so a `fail-on` edited down to ERR in the workflow alone lands green with the whole suite passing. The new test reads both values back out of the YAML and needs no engine, so it runs in CI. Also drops the rest of the header's not-committed-yet wording (the merge before this one did the same for COMPATIBILITY.md), and records in CLAUDE.md that the remote's workflow-path rejection is per commit diff, so merging a main that changed build.yml is pushable. * docs: restore the two CLAUDE.md bullets on the oasdiff gate The merge in 94e6ed5 hit a conflict where main's two new semver bullets and this branch's two oasdiff bullets both landed under the Docker one, and took main's side whole, so the gate this PR adds ended up documented nowhere in CLAUDE.md. Put them back in place, unchanged, alongside the semver pair. Also notes on the workflow-push bullet that the App did push a merge carrying main's build.yml changes (ce0fc59 on this branch), so a sync is worth trying before treating it as maintainer-only. * docs: record the wasm browser-test flake and what its error actually means Hit on this branch: the same docs-only sha passed all three browser jobs at 07:45 and failed at 07:48. The visible error, `missing field 'chunk'`, is the test runner choking on a truncated webdriver reply, so it reads like a harness version mismatch; the actual cause is the renderer timeout logged one line above it. Also notes that only chrome failed and the other two were cancelled by the fail-fast matrix but reported as failures, and that the App cannot re-run a workflow. * test(pg-pkg): pin the oasdiff action ref alongside the gate's inputs `the_workflow_step_matches_the_pinned_inputs` read `fail-on` and `include-checks` back out of the workflow but not the action ref, which is the third thing the pinned verdicts depend on: `breaking@0ab8ad2` is v0.1.10, whose Dockerfile is `FROM tufin/oasdiff:v1.26.1`, and every verdict in this file was measured against v1.26.1. So bumping the action left all three tests green while the verdicts quietly stopped describing CI -- the same fail-open the `fail-on` pin exists to prevent, and the file's header claims its verdict is CI's verdict. Assert the workflow contains the ref too. Verified by bumping the ref in the workflow: the test fails and names it. * build: keep api-description.yaml LF on Windows checkouts pg-pkg/tests/api_gate.rs locates blocks in the spec with multi-line raw-string anchors. rustc normalises CRLF to LF inside raw strings, but fs::read_to_string of the spec keeps whatever git wrote, so on a Windows checkout (core.autocrlf=true is the Git for Windows default) every multi-line anchor misses. Reproduced rather than argued: converting the spec to CRLF fails every_mutation_still_applies with "anchor is not unique in the spec", i.e. the one gate test that runs without the external engine, so a Windows contributor gets a red suite on a clean tree with a message blaming the spec. CI is ubuntu/macos only, so it never catches this. Mark the spec -text, which keeps stored bytes equal to checked-out bytes in both directions. --------- Co-authored-by: dobby-yivi-agent[bot] <275734547+dobby-yivi-agent[bot]@users.noreply.github.com> Co-authored-by: Claude Opus 5 <noreply@anthropic.com> Co-authored-by: Ruben Hensen <ruben.hensen@protonmail.com>
1 participant
Closes #209, closes #210 (Phase 1 of EPIC #201) — and goes one step further than pinning the contract: it fixes the leniency the contract would otherwise have had to document as a flaw.
The behavioral change: unknown request fields are rejected
Previously the parser silently ignored unknown fields, so a misspelled
v(vaule) widened the disclosure the caller intended — a silent intent-weakening in a key-issuance API. Now:IrmaAuthRequest,DisclosureAttribute,SigningKeyRequestare#[serde(deny_unknown_fields)]→ a typo is a 400 naming the field, not a reinterpretation.ConItem's untagged deserialize is replaced with a manual visitor (object vs array discriminates the variants), so clients get serde's precise inner error — ``unknown fieldvaule, expected one of `t`, `v`, `optional``` — instead of "did not match any variant".{error, message}envelope (previously actix's plain-text default — the spec and reality disagreed).identity::Attributestays lenient deliberately: it's shared with the bincode wire format, the wasm/FFI seal boundary and response policies, and its values aren't load-bearing in request paths (documented in the spec).Breaking note: any client sending extra request fields gets 400s after this. All clients are first-party and none send extra fields (pg-js/dotnet/harness send exact shapes); the two PoC deployments should update/verify — the error message tells them exactly what to fix if anything surfaces.
The pinned contract
pg-pkg/api-description.yaml(PKG v2 OpenAPI spec #209): the full v2 OpenAPI (all endpoints incl./statusevents/{token}, both auth modes, error envelope, rate limits, the/v2/{irma|request}alias policy). Lints clean (redocly).pg-pkg/schemas/irma-auth-request.schema.json(Condiscon request JSON Schema + canonical examples #210): the condiscon request as JSON Schema, nowadditionalProperties: false— schema and parser agree with no caveats.pg-pkg/tests/contract_examples.rs): everyvalid/example must parse, everyinvalid/must be rejected — including the new typo vectors (unknown-attribute-field.json,unknown-top-level-field.json). Commentary lives inexamples/README.md(JSON comments would now rightly be rejected — the old$comments only parsed thanks to the leniency this PR removes). Covers the legacy flat shape, the website's name disjunction, the optional empty-conjunction convention with the irmamobile#360 ordering workaround, and the classic nesting mistakes.test_unknown_request_field_yields_400_envelopeproves the 400 envelope + field-naming end-to-end through the actix extractor.Tests: pg-core 57 + api tests, pg-pkg 58 + 2 contract tests — all green.
Follow-up (#212, postguard-e2e): validate live PKG responses against the OpenAPI and the examples against the schema, closing the loop schema ↔ parser ↔ running server.